feat(ui5): Add QUnit best-practices skill#80
Conversation
Adds the ui5-best-practices-qunit skill covering coding standards for OpenUI5/SAPUI5 unit test files. Follows the same structure as the OPA5 skill: a lean SKILL.md router plus three focused reference files loaded on demand by task type (writing new tests, modernizing existing ones, async patterns). - SKILL.md: trigger description with literal user phrases, core rules table, quick-reference checklist - references/writing-new-tests.md: module structure, AAA pattern, test naming, helpers, file setup (/*global QUnit */, sap.ui.define imports) - references/modernizing-tests.md: var/const/let, bind, assert.async, Core.applyChanges, sinon sandbox, assert.expect, import cleanup, encoding fix, what-not-to-change guard table - references/async-patterns.md: nextUIUpdate vs Core.applyChanges decision table (incl. fake-timer exceptions), waitForEvent, waitForRendering via addEventDelegate, when not to convert assert.async - README.md: adds ui5-best-practices-qunit section - plugin.json / .github/plugin/plugin.json: adds "qunit" keyword - All files ISO 8859-1 compliant (no em dashes or non-ASCII) JIRA: BGSOFUIPIRIN-7067
| - **i18n** - Bind all user-facing strings to the i18n model; never hardcode | ||
| - **Actions** - Use the `actions` property for links and interactions; never inline `<a>` tags or hand-roll URL handlers | ||
|
|
||
| #### ui5-best-practices-qunit |
There was a problem hiding this comment.
please move this section below the opa5 ones, so it's sorted alphabetically
There was a problem hiding this comment.
@codeworrior Could you have a look at this QUnit best practices, please?
codeworrior
left a comment
codeworrior
left a comment
There was a problem hiding this comment.
Overall LGTM, but IMO, there are still a few things to be clarified / added.
| | Shared helper functions (`renderObject`, `waitForUIUpdates`) used by many tests | Keep them synchronous so callers can reason about DOM state without async coordination. | | ||
| | Inside a `load` event callback that must flush a subsequent `invalidate()` synchronously | `Core.applyChanges()` must be called inside the callback. | | ||
|
|
||
| Note the reason in the commit message when keeping `Core.applyChanges()` so future reviewers do not flag it. |
There was a problem hiding this comment.
I'm not happy with this section (Keep Core.applyChanges()). It's valid in the sense that there's a high risk of failure. But I'm nevertheless missing some aspects:
- when
Core.applyChangesis not removed, then the test won't work with legacy-free UI5. I would therefore expect that the developer gets informed that manual effort is needed. or do we in general rely on the remaining linter issues? - for the fake timer reason, I'm missing the hint hat nextUIUpdate can be given a clock instance. It will then tick the clock on its own. This doesn't fix everything and it might be hard to describe the cases that can be fixed (@loginger any hints?)
- maybe I just missed it, but shouldn't it be mentioned that nextUIUpdate exists twice and only
sap/ui/test/nextUIUpdatemust be used?
I'm also not sure with the "so future reviewers do not flag it" part. Is it just to avoid repeated attempts to fix it? Wouldn't a marker (comment) per occurrence be more helpful? 20 different places might have 20 different reasons. Mapping this between commit message and code might be cumbersome.
Last but not least, we should consider in the framework team whether we want to document (but still deprecate) the nextUIUpdate.runSync. It contrast to Core.applyChanges, it is intended for use in test code only. In production code none of the methods must be used.
| const oSandbox = sinon.createSandbox(); | ||
| ``` | ||
|
|
||
| When the QUnit-sinon integration is already active, `this.stub()`, `this.spy()`, and `this.clock` are available directly on the QUnit context - a manual sandbox is not needed at all. |
There was a problem hiding this comment.
Wouldn't it be a good guideline to avoid a mixture of bridge (QUnit-sinon integration) and explicitly created sandbox? If sinon is used via this, no sandbox should be created. Migration fro sandbox to bridge is however difficult when verifyAndRestore is called within the test. Not sure whether the bridge exposes this.
|
|
||
| ## 7. Remove unused imports | ||
|
|
||
| After replacing all `Core.applyChanges()` calls, remove `sap/ui/core/Core` from the `sap.ui.define` array and function parameters - unless `Core` is still used elsewhere (e.g. `Core.byId`, `Core.getConfiguration`). |
There was a problem hiding this comment.
Please note that a dependency to Core should also be kept, when there's a sap.ui.getCore() call anywhere in the test. Strictly spoken, this is not an obvious usage of Core.
Sometimes, it's not called Core, but oCore or CoreInstance or theCore. Can AI safely derive this from mentioning Core or should be rather say "from the import parameter representing sap/ui/coreCore" or similar?
|
|
||
| ## 8. Fix non-ASCII characters | ||
|
|
||
| Replace em dashes (U+2014) and other non-ASCII characters in comments with plain ASCII hyphens. Files must be ISO 8859-1 compliant. |
There was a problem hiding this comment.
"must be" is a very strong wording. Technically, we still say "utf-8" is required, not ISO-8859-1. But you're right that we got issues from time to time with UTF-8 encoding, especially, when the <meta charset> tag was missing in the HTML.
Maybe it's safer to follow the ISO-8859-1 idea.
|
|
||
| Find violations: | ||
| ```bash | ||
| grep -Pn '[^\x00-\x7E]' src/<library>/test/**/*.qunit.js |
There was a problem hiding this comment.
Mhmm, where is this file path pattern used? In S/4 reuse libs, it's rather test/<library>/**/*.qunit.js, sometimes even w/o the <library> part. In apps, it is src/main/webapp/test/**/*.qunit.js or webapp/test/**/*.qunit.js.
|
|
||
| ## 7. File setup | ||
|
|
||
| - Add `/*global QUnit */` as the first line so ESLint recognises the QUnit global without requiring an explicit import. |
There was a problem hiding this comment.
Foundation team will be working on a feature to make the use of global obsolete (CPOUI5FOUNDATION-1204).
As long as this isn't available, sinon should be used the same way, not via dependency. Or, when used via dependency, then neither the bridge must be used nor must sinon be configured in the test starter (all manual, so to say)
|
|
||
| Verify encoding before committing: | ||
| ```bash | ||
| grep -Pn '[^\x00-\x7E]' src/<library>/test/**/*.qunit.js |
There was a problem hiding this comment.
same here, pattern doesn't look familiar.
| - [ ] No `.bind(this)` - use arrow functions for callbacks that do not need their own `this` | ||
| - [ ] No `assert.async()` in simple cases - use `async function` + `await new Promise(...)` | ||
| - [ ] Every `async` test has `assert.expect(N)` | ||
| - [ ] No `sinon.sandbox.create()` - use `sinon.createSandbox()` |
| @@ -0,0 +1,50 @@ | |||
| --- | |||
| name: ui5-best-practices-qunit | |||
There was a problem hiding this comment.
I'm missing hints what needs to be done when migrating away from QUnit 1 and globals. With QUnit 1 (sap/ui/thirdparty/qunit.js or a test starter configuration with qunit/version: 1), it was possible to use test, asyncTest, and all assertions (ok, equal, strictEqual, ...) as globals. Furthermore, the expected assertions have been a parameter of the test method etc.
3 participants
Adds the ui5-best-practices-qunit skill covering coding standards for OpenUI5/SAPUI5 unit test files. Follows the same structure as the OPA5 skill: a lean SKILL.md router plus three focused reference files loaded on demand by task type (writing new tests, modernizing existing ones, async patterns).
JIRA: BGSOFUIPIRIN-7067